-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling of multiple compilation errors #1011
Fix handling of multiple compilation errors #1011
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any example of the CLI output?
With modern logs for Framework v3 release (which I plan to propose with next PR), without this change, issue exposes as: (Note the another stats error log, after command reported the final error). After this change, all the stats logged will appear before final command error output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which I plan to propose with next PR
How many did you plan to propose? :D
I'm joking, thanks for your contributions!
@@ -87,7 +87,7 @@ function webpackCompile(config, logStats, ServerlessError) { | |||
_.forEach(stats, compileStats => { | |||
logStats(compileStats); | |||
if (compileStats.hasErrors()) { | |||
throw new ServerlessError('Webpack compilation error, see stats above'); | |||
throw _.assign(new ServerlessError('Webpack compilation error, see stats above'), { stats: compileStats }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not actually a change request, but rather grabbing the chance to discuss with people who cares about the project.
When I am experimenting the reverting of MultiCompile/ combined entries approach, I can't help but think of a complete rewrite of this project with TypeScript. The monkey patching approach brought down from Serverless V1 makes debugging and error tracing a bit tricky in many cases.
The effort is not small though, do you think it's worth doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vicary thanks for comment, but do you see this discussion specific to this contribution?
This PR is about grouping all compilation errors into one error, and not maintaining a situation where first one is reported, and others jump out aside (after Framework reports that command processing is finalized)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more a global discussion not specific to your contribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing that you're adding stats
to the ServerlessError
object, I just expanded the topic a bit.
But definitely not a blocker for this issue, just general discussion.
337a90d
to
73c20cb
Compare
Currently, when compiling multiple bundles, the first error is reported, and the rest are reported in the air (after the command is reported as finalized).
This patch fixes, so all errors are acknowledged, and that command reports final error when compilations of all bundles finalizes